Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(MWPW-142267): Merch What's Included and Merch Mnemonic List (TwP) #4

Merged
merged 4 commits into from
Jul 9, 2024

Conversation

Axelcureno
Copy link
Member

@Axelcureno Axelcureno commented Jun 20, 2024

Introduces Merch What's Included and Merch Mnemonic List web components for TwP What's included screen.
Resolves: MWPW-142267
Test link: https://mwpw-142267--milo--adobecom.hlx.page/drafts/axel/block-samples/twp/illustrator#twpmodal
Migrated from https://git.corp.adobe.com/wcms/tacocat.js/pull/572
d8d35034-689b-47b0-8079-a935ec774a8a

How it works in Milo
In merch-twp block
Given a merch-twp block, a third separator --- is expected and then below a link to the what's included screen modal content.
fef4cdb1-2418-4218-b6d5-50307bbbf348

In merch-card block
A link with name "merch-whats-included" is expected (link text is authorable)
e56673f8-7c12-4a54-a1e1-0872c7a81fed

This PR also introduces 2 new web components, which are:

merch-whats-included
82c91645-1ef5-42e1-8034-79eaeac77f59
Contains link of whats-included modal content (works as an inline fragment) and lets the author configure number of rows in mobile to display before "+ See more".

merch-mnemonic-list
c7ccda5c-6ccf-458f-9392-fd4eb1b5648e

Contains content of what's included screen.

Assumptions
What's included is an experience only applicable to a CC All Apps merch card.
Supports unlimited number of columns
Supports configurable number of rows before displaying "+ See more" in mobile.

Preview URLs:

URL for testing:

Copy link

aem-code-sync bot commented Jun 20, 2024

Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
In case there are problems, just click a checkbox below to rerun the respective action.

  • Re-run PSI checks
  • Re-sync branch
Commits

Copy link
Collaborator

@yesil yesil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overal I like your approach with [slot="merch-whats-included"]

  1. There are some visual issues on mobile.

image

  1. expecting a fragment with a specific name feels a bit limiting to me. Couldn't we use a #merch-whats-included hash or something similar?

  2. How to we close and go back to TWP without closing the modal?

</merch-mnemonic-list>
<merch-mnemonic-list>
<div slot="icon">
<merch-icon size="s" src="https://www.adobe.com/content/dam/shared/images/product-icons/svg/premiere.svg" alt="Premiere Pro"></merch-icon>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need the div? can we add slot="icon" directly on merch-icon?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@Axelcureno
Copy link
Member Author

Axelcureno commented Jul 9, 2024

Overal I like your approach with [slot="merch-whats-included"]

  1. There are some visual issues on mobile.

image

  1. expecting a fragment with a specific name feels a bit limiting to me. Couldn't we use a #merch-whats-included hash or something similar?
  2. How to we close and go back to TWP without closing the modal?

@yesil Please find my answers below.

  1. Fixed.
    2: Using a hash will make Milo treat the content as Modal which complicates things and would require a "hacky" approach to not update the URL and capture the click event of such modal. Using query parameters also doesn't work with fragments since those are stripped by milo. In my example it is not specifically tied to that name, it could be a different name as long as the name contains 'merch-whats-included'. Another thing to keep in mind is that the end user will not see this file name at all (unless inspecting the code). Please suggest.

  2. By capturing the close modal event and toggling if the fragment content visibility. I have started a thread in #milo-dev

@3ch023
Copy link
Collaborator

3ch023 commented Jul 9, 2024

@Axelcureno to make the PSI check green you can just provide a fake link, with your branch, you can check the example green pr here:
#19

@Axelcureno Axelcureno self-assigned this Jul 9, 2024
@Axelcureno
Copy link
Member Author

Axelcureno commented Jul 9, 2024

Copy link

aem-code-sync bot commented Jul 9, 2024

Page Scores Audits Google
/?martech=off PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@Axelcureno Axelcureno requested a review from yesil July 9, 2024 14:39
Copy link
Collaborator

@yesil yesil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's merge for now, but prevent GWP from authoring yet with this

entryPoints: ['./src/merch-twp-d2p.js'],
bundle: true,
banner,
outfile: './lib/merch-twp-d2p.js',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merch-twp-d2p is already being build below.
also ./lib has changed to ./libs

Comment on lines 104 to 120
build({
entryPoints: ['./src/merch-whats-included.js'],
bundle: true,
banner,
outfile: './lib/merch-whats-included.js',
format: 'esm',
plugins: [rewriteImports()],
external: ['lit'],
}),
build({
entryPoints: ['./src/merch-mnemonic-list.js'],
bundle: true,
banner,
outfile: './lib/merch-mnemonic-list.js',
format: 'esm',
plugins: [rewriteImports()],
external: ['lit'],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these build entries are not needed.

@yesil yesil merged commit 67de0c8 into main Jul 9, 2024
4 of 5 checks passed
rohitsahu pushed a commit that referenced this pull request Jul 10, 2024
…into rosahu/MWPW-147172

* 'rosahu/MWPW-147172' of https://github.com/adobecom/mas:
  feat(MWPW-142267): Merch What's Included and Merch Mnemonic List (TwP) (#4)
  MWPW-153962: update /lib to /libs (#30)
  MWPW-153658 audit script (#25)
  MWPW-153962: support maslibs approach (#28)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants